From ce14ff2f77ce26ed2fc8a2005e700bc930ea6c28 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Wed, 29 Oct 2014 11:42:45 -0700
Subject: [PATCH] Be sure to update all packages from a git source

With the recent resolve rewrite, `cargo update -p foo` would only update one
package in a git repository, even if the repository provided many packages.
Cargo does not currently support depending on the same git repository source
with different precise revisions, and this would cause errors down the line
depending on what happened.

This adds a fix to the `resolve_with_previous` method to ensure that any
non-registry sources being updated will not have any locked packages inside them
which would result in an invalid lockfile.
---
 src/cargo/ops/resolve.rs             | 34 ++++++++++++++---
 tests/test_cargo_compile_git_deps.rs | 55 ++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index 62b6e9f3f..70c2c1dfa 100644
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -1,6 +1,6 @@
 use std::collections::{HashMap, HashSet};
 
-use core::{Package, PackageId};
+use core::{Package, PackageId, SourceId};
 use core::registry::PackageRegistry;
 use core::resolver::{mod, Resolve};
 use ops;
@@ -39,6 +39,26 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
     let root = package.get_package_id().get_source_id().clone();
     try!(registry.add_sources(&[root]));
 
+    // Here we place an artificial limitation that all non-registry sources
+    // cannot be locked at more than one revision. This means that if a git
+    // repository provides more than one package, they must all be updated in
+    // step when any of them are updated.
+    //
+    // TODO: This seems like a hokey reason to single out the registry as being
+    //       different
+    let mut to_avoid_sources = HashSet::new();
+    match to_avoid {
+        Some(set) => {
+            for package_id in set.iter() {
+                let source = package_id.get_source_id();
+                if !source.is_registry() {
+                    to_avoid_sources.insert(source);
+                }
+            }
+        }
+        None => {}
+    }
+
     let summary = package.get_summary().clone();
     let summary = match previous {
         Some(r) => {
@@ -63,15 +83,15 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
             //    ranges. To deal with this, we only actually lock a dependency
             //    to the previously resolved version if the dependency listed
             //    still matches the locked version.
-            for node in r.iter().filter(|p| keep(p, to_avoid)) {
+            for node in r.iter().filter(|p| keep(p, to_avoid, &to_avoid_sources)) {
                 let deps = r.deps(node).into_iter().flat_map(|i| i)
-                            .filter(|p| keep(p, to_avoid))
+                            .filter(|p| keep(p, to_avoid, &to_avoid_sources))
                             .map(|p| p.clone()).collect();
                 registry.register_lock(node.clone(), deps);
             }
 
             let map = r.deps(r.root()).into_iter().flat_map(|i| i).filter(|p| {
-                keep(p, to_avoid)
+                keep(p, to_avoid, &to_avoid_sources)
             }).map(|d| {
                 (d.get_name(), d)
             }).collect::<HashMap<_, _>>();
@@ -92,9 +112,11 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
     }
     return Ok(resolved);
 
-    fn keep<'a>(p: &&'a PackageId, to_avoid: Option<&HashSet<&'a PackageId>>)
+    fn keep<'a>(p: &&'a PackageId,
+                to_avoid_packages: Option<&HashSet<&'a PackageId>>,
+                to_avoid_sources: &HashSet<&'a SourceId>)
                 -> bool {
-        match to_avoid {
+        !to_avoid_sources.contains(&p.get_source_id()) && match to_avoid_packages {
             Some(set) => !set.contains(p),
             None => true,
         }
diff --git a/tests/test_cargo_compile_git_deps.rs b/tests/test_cargo_compile_git_deps.rs
index 82d166a86..6f8089486 100644
--- a/tests/test_cargo_compile_git_deps.rs
+++ b/tests/test_cargo_compile_git_deps.rs
@@ -1508,3 +1508,58 @@ Updating git repository `{}`
 {compiling} project [..]
 ", dep2.url(), compiling = COMPILING)));
 })
+
+test!(update_one_source_updates_all_packages_in_that_git_source {
+    let dep = git_repo("dep", |project| {
+        project.file("Cargo.toml", r#"
+            [package]
+            name = "dep"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies.a]
+            path = "a"
+        "#)
+        .file("src/lib.rs", "")
+        .file("a/Cargo.toml", r#"
+            [package]
+            name = "a"
+            version = "0.5.0"
+            authors = []
+        "#)
+        .file("a/src/lib.rs", "")
+    }).assert();
+
+    let p = project("project")
+        .file("Cargo.toml", format!(r#"
+            [project]
+            name = "project"
+            version = "0.5.0"
+            authors = []
+            [dependencies.dep]
+            git = '{}'
+        "#, dep.url()).as_slice())
+        .file("src/main.rs", "fn main() {}");
+
+    p.build();
+    assert_that(p.process(cargo_dir().join("cargo")).arg("build"),
+                execs().with_status(0));
+
+    let repo = git2::Repository::open(&dep.root()).unwrap();
+    let rev1 = repo.revparse_single("HEAD").unwrap().id();
+
+    // Just be sure to change a file
+    File::create(&dep.root().join("src/lib.rs")).write_str(r#"
+        pub fn bar() -> int { 2 }
+    "#).assert();
+    add(&repo);
+    commit(&repo);
+
+    assert_that(p.process(cargo_dir().join("cargo")).arg("update")
+                 .arg("-p").arg("dep"),
+                execs().with_status(0));
+    let lockfile = File::open(&p.root().join("Cargo.lock")).read_to_string()
+                                                           .unwrap();
+    assert!(!lockfile.as_slice().contains(rev1.to_string().as_slice()),
+            "{} in {}", rev1, lockfile);
+})
-- 
2.30.2